Support omitzero tags in optionalfields linter #115

Karthik-K-N · 2025-07-03T17:08:44Z

Add support considerin omitzero json tag in optionalfield linter.

TODO:

Update requiredfield linter to consider omitzero tag
Add more tests

k8s-ci-robot · 2025-07-03T17:08:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Karthik-K-N
Once this PR has been reviewed and has the lgtm label, please assign jpbetz for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Karthik-K-N · 2025-07-03T17:10:27Z

@JoelSpeed I spent some time understanding the linter and tried adding support for conisdering omitzero JSON tag. Please let me know what other scenarios needs to be covered.

Is it required to add a configuration to suggest/fix? like omitemtpy tag

/cc @sbueringer

sbueringer · 2025-07-04T03:32:03Z

This PR would be very very helpful for CAPI. Currently I have a lot of excludes for omitzero cases which is a bit error prone :)

Thx for working on this!

pkg/analysis/helpers/extractjsontags/analyzer.go

pkg/analysis/optionalfields/testdata/src/b/a.go

sivchari · 2025-07-04T04:24:46Z

pkg/analysis/optionalfields/testdata/src/b/a.go

Please add the field with only omitzero tag.

Added, but whats the expectation, Currently linter suggests to add omitempty tag, Should that be honored or should we make changes to avoid that suggestion when omitzero present on optional tags?

Upstream guidance is still that omitempty should be everywhere, but it's useful to have a case where it is omitzero only, as I would expect that in the case where omitempty is being ignored, this change should still work

pkg/analysis/helpers/extractjsontags/analyzer.go

pkg/analysis/optionalfields/testdata/src/b/a.go

JoelSpeed · 2025-07-04T08:35:38Z

I wouldn't worry about the requiredfields linter right now, that needs a bit of an overhaul

pkg/analysis/optionalfields/testdata/src/b/a.go

JoelSpeed

So I think we are going to need to expand the config to have similar omitempty style behaviour, but with omitzero on structs only for now

I'd also like to see more test cases especially with the different configurations

E.g. if the omitzero isn't present on a struct, do we still recommend it to be a pointer? Or do we say now, no, this should be omtizero (I think the latter is preferable so I'd expect to see some test cases updated to reflect that, right?)

JoelSpeed · 2025-07-07T09:29:17Z

pkg/analysis/optionalfields/analyzer.go

-		// Since we absolutely have to add the omitempty tag, we can report it as a suggestion.
-		reportShouldAddOmitEmpty(pass, field, config.OptionalFieldsOmitEmptyPolicySuggestFix, fieldName, "field %s is optional and does not allow the zero value. It must have the omitempty tag.", jsonTags)
+		if isStruct && hasOmitZero {
+			// The struct field need not have omitempty tag if it does not have a valid zero value and has omitzero tag.


I think we are going to need similar omitzero behaviour/configuration as to what we have for omitempty. The logic below in the else clause is important for when the object is a struct, but does not have omitzero, we would need to suggest it.

At the moment, I would only recommend the omitzero checks/recommendations to look at structs

JoelSpeed · 2025-07-07T09:32:21Z

pkg/analysis/optionalfields/analyzer.go

@@ -300,7 +314,7 @@ func getStructZeroValue(pass *analysis.Pass, structType *ast.StructType) string
 	for _, field := range structType.Fields.List {
 		fieldTagInfo := jsonTagInfo.FieldTags(field)

-		if fieldTagInfo.OmitEmpty {
+		if fieldTagInfo.OmitEmpty || fieldTagInfo.OmitZero {


I think we can only rely on omitzero when the field isn't a pointer, can we check that here?

JoelSpeed · 2025-07-07T09:48:46Z

pkg/analysis/optionalfields/testdata/src/b/a.go

+	// structWithOnlyOmitZeroTag is a struct field with a minimum number of properties and has only omitzero tag.
+	// +kubebuilder:validation:MinProperties=1
+	// +optional
+	StructWithOnlyOmitZeroTag B `json:"structWithOnlyOmitZeroTag,omitzero"`


IMO, if we aren't using the ignore policy on omitempty, we should add omitempty as well as omitzero to be consistent with other types

Karthik-K-N · 2025-07-07T10:27:01Z

So I think we are going to need to expand the config to have similar omitempty style behaviour, but with omitzero on structs only for now

I'd also like to see more test cases especially with the different configurations

E.g. if the omitzero isn't present on a struct, do we still recommend it to be a pointer? Or do we say now, no, this should be omtizero (I think the latter is preferable so I'd expect to see some test cases updated to reflect that, right?)

Yep sure, I will update it

Karthik-K-N · 2025-07-09T15:36:31Z

So I think we are going to need to expand the config to have similar omitempty style behaviour, but with omitzero on structs only for now

I'd also like to see more test cases especially with the different configurations

E.g. if the omitzero isn't present on a struct, do we still recommend it to be a pointer? Or do we say now, no, this should be omtizero (I think the latter is preferable so I'd expect to see some test cases updated to reflect that, right?)

Hey, I was implementing this and I have a functional question?

Case 1:

Pointer Policy: Whenrequired
Omitzero policy: Ignore
OmitEmpty Policy: SuggestFix
Field: Optional struct with NoValidZeroValue

Result: Struct should be made Pointer.

Case 2:

Pointer Policy: Whenrequired
Omitzero policy: SuggestFix
OmitEmpty Policy: SuggestFix
Field: Optional struct with NoValidZeroValue

Result: Struct should be tagged with omitzero (no need to be a pointer)

Is my understanding correct

k8s-ci-robot · 2025-07-09T15:36:42Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

JoelSpeed · 2025-07-09T16:27:34Z

Case 1:

Pointer Policy: Whenrequired
Omitzero policy: Ignore
OmitEmpty Policy: SuggestFix
Field: Optional struct with NoValidZeroValue

Result: Struct should be made Pointer.

Yes, that would work, though I think in this case I would force the omitzero rather than making it a pointer. I believe in the case where we ignore omitempty there are certain cases where we have said that actually we still force the omitempty right?

JoelSpeed · 2025-07-09T16:30:41Z

So I think in this case I'd probably be doing something like

kube-api-linter/pkg/analysis/optionalfields/analyzer.go

Lines 186 to 189 in 029010b

    
           // The zero value would not be accepted, so the field needs to have omitempty. 
        
           // Force the omitempty policy to suggest a fix. We can only get to this function when the policy is configured to Ignore. 
        
           // Since we absolutely have to add the omitempty tag, we can report it as a suggestion. 
        
           reportShouldAddOmitEmpty(pass, field, OptionalFieldsOmitEmptyPolicySuggestFix, fieldName, "field %s is optional and does not allow the zero value. It must have the omitempty tag.", jsonTags)

Does that make sense? Does it then override any point having the ability to ignore the omitzero?

(This is a really good question btw)

Karthik-K-N · 2025-07-11T03:22:43Z

Got the points thanks, Then I think it would be better we document these behavior, My only concern is user should not get confused why still its recommending the tags though the policy is ignore.

Karthik-K-N · 2025-07-11T03:23:18Z

So I think in this case I'd probably be doing something like

kube-api-linter/pkg/analysis/optionalfields/analyzer.go

Lines 186 to 189 in 029010b

// The zero value would not be accepted, so the field needs to have omitempty.

// Force the omitempty policy to suggest a fix. We can only get to this function when the policy is configured to Ignore.

// Since we absolutely have to add the omitempty tag, we can report it as a suggestion.

reportShouldAddOmitEmpty(pass, field, OptionalFieldsOmitEmptyPolicySuggestFix, fieldName, "field %s is optional and does not allow the zero value. It must have the omitempty tag.", jsonTags)

Does that make sense? Does it then override any point having the ability to ignore the omitzero?

(This is a really good question btw)

I will get back for this soon.

Support omitzero tags in optionalfields linter

d5f8bf2

k8s-ci-robot requested review from everettraven and sivchari July 3, 2025 17:08

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 3, 2025

sivchari reviewed Jul 4, 2025

View reviewed changes

JoelSpeed reviewed Jul 4, 2025

View reviewed changes

pkg/analysis/helpers/extractjsontags/analyzer.go Outdated Show resolved Hide resolved

pkg/analysis/optionalfields/testdata/src/b/a.go Outdated Show resolved Hide resolved

JoelSpeed reviewed Jul 4, 2025

View reviewed changes

pkg/analysis/optionalfields/testdata/src/b/a.go Outdated Show resolved Hide resolved

Address review comments

8191661

Karthik-K-N force-pushed the optionalfields branch from 4a4e30f to 8191661 Compare July 7, 2025 06:18

JoelSpeed reviewed Jul 7, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2025

Support omitzero tags in optionalfields linter #115

Are you sure you want to change the base?

Support omitzero tags in optionalfields linter #115

Conversation

Karthik-K-N commented Jul 3, 2025

Uh oh!

k8s-ci-robot commented Jul 3, 2025

Uh oh!

Karthik-K-N commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbueringer commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sivchari Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

Karthik-K-N Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JoelSpeed commented Jul 4, 2025

Uh oh!

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Karthik-K-N commented Jul 7, 2025

Uh oh!

Karthik-K-N commented Jul 9, 2025

Uh oh!

k8s-ci-robot commented Jul 9, 2025

Uh oh!

JoelSpeed commented Jul 9, 2025

Uh oh!

JoelSpeed commented Jul 9, 2025

Uh oh!

Karthik-K-N commented Jul 11, 2025

Uh oh!

Karthik-K-N commented Jul 11, 2025

Uh oh!

Uh oh!

Karthik-K-N commented Jul 3, 2025 •

edited

Loading

sbueringer commented Jul 4, 2025 •

edited

Loading